Repository navigation
Conversation
Member
|
@llvm/pr-subscribers-bolt @llvm/pr-subscribers-pgo Author: David Zbarsky (dzbarsky) ChangesPatch is 64.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/177868.diff 3 Files Affected:
diff --git a/llvm/tools/llvm-profdata/CMakeLists.txt b/llvm/tools/llvm-profdata/CMakeLists.txt
index e5aa858f3d39c..2a98ed0be0fb5 100644
--- a/llvm/tools/llvm-profdata/CMakeLists.txt
+++ b/llvm/tools/llvm-profdata/CMakeLists.txt
@@ -2,14 +2,21 @@ set(LLVM_LINK_COMPONENTS
Core
Object
ProfileData
+ Option
Support
)
+set(LLVM_TARGET_DEFINITIONS Opts.td)
+tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+add_public_tablegen_target(ProfdataOptsTableGen)
+
add_llvm_tool(llvm-profdata
llvm-profdata.cpp
DEPENDS
intrinsics_gen
+ ProfdataOptsTableGen
+ GENERATE_DRIVER
)
target_link_libraries(llvm-profdata PRIVATE LLVMDebuginfod)
diff --git a/llvm/tools/llvm-profdata/Opts.td b/llvm/tools/llvm-profdata/Opts.td
new file mode 100644
index 0000000000000..8803af9a10123
--- /dev/null
+++ b/llvm/tools/llvm-profdata/Opts.td
@@ -0,0 +1,314 @@
+include "llvm/Option/OptParser.td"
+
+def Show : SubCommand<"show",
+ "Takes a profile data file and displays the profiles. "
+ "See detailed documentation in "
+ "https://llvm.org/docs/CommandGuide/llvm-profdata.html"
+ "#profdata-show">;
+def Order : SubCommand<"order",
+ "Reads temporal profiling traces from a profile and "
+ "outputs a function order that reduces page faults. "
+ "See detailed documentation in "
+ "https://llvm.org/docs/CommandGuide/llvm-profdata.html"
+ "#profdata-order">;
+def Overlap : SubCommand<"overlap",
+ "Computes the overlap between two profiles. See "
+ "detailed documentation in "
+ "https://llvm.org/docs/CommandGuide/llvm-profdata.html"
+ "#profdata-overlap">;
+def Merge : SubCommand<"merge",
+ "Takes several profiles and merge them together. See "
+ "detailed documentation in "
+ "https://llvm.org/docs/CommandGuide/llvm-profdata.html"
+ "#profdata-merge">;
+
+def help : Flag<["-","--"], "help", [Show, Order, Overlap, Merge]>,
+ HelpText<"Display this help">;
+def help_hidden : Flag<["--"], "help-hidden", [Show, Order, Overlap, Merge]>,
+ HelpText<"Display all help, including hidden options">;
+def version : Flag<["--"], "version", [Show, Order, Overlap, Merge]>,
+ HelpText<"Display the version">;
+
+def output : JoinedOrSeparate<["-","--"], "output",
+ [Show, Order, Overlap, Merge]>,
+ MetaVarName<"<output>">,
+ HelpText<"Output file">;
+def : JoinedOrSeparate<["-"], "o", [Show, Order, Overlap, Merge]>,
+ Alias<output>, HelpText<"Alias for --output">;
+
+def instr : Flag<["-","--"], "instr", [Show, Overlap, Merge]>,
+ HelpText<"Instrumentation profile (default)">;
+def sample : Flag<["-","--"], "sample", [Show, Overlap, Merge]>,
+ HelpText<"Sample profile">;
+def memory : Flag<["-","--"], "memory", [Show]>,
+ HelpText<"MemProf memory access profile">;
+
+def max_debug_info_correlation_warnings
+ : JoinedOrSeparate<["-","--"], "max-debug-info-correlation-warnings",
+ [Show, Merge]>,
+ HelpText<"Maximum number of warnings to emit when correlating profile "
+ "from debug info (0 = no limit)">,
+ MetaVarName<"<n>">;
+def profiled_binary
+ : JoinedOrSeparate<["-","--"], "profiled-binary", [Show, Merge]>,
+ HelpText<"Path to binary from which the profile was collected.">,
+ MetaVarName<"<binary>">;
+def debug_info
+ : JoinedOrSeparate<["-","--"], "debug-info", [Show, Merge]>,
+ HelpText<"For show, read and extract profile metadata from debug info. "
+ "For merge, correlate the raw profile using the provided "
+ "debug info.">,
+ MetaVarName<"<file>">;
+def binary_file
+ : JoinedOrSeparate<["-","--"], "binary-file", [Merge]>,
+ HelpText<"Use the provided unstripped binary to correlate the raw "
+ "profile.">,
+ MetaVarName<"<file>">;
+def debug_file_directory
+ : JoinedOrSeparate<["-","--"], "debug-file-directory",
+ [Show, Order, Overlap, Merge]>,
+ HelpText<"Directories to search for object files by build ID">,
+ MetaVarName<"<dir>">;
+def debuginfod : Flag<["-","--"], "debuginfod", [Merge]>,
+ HelpText<"Enable debuginfod">, Flags<[HelpHidden]>;
+def correlate : JoinedOrSeparate<["-","--"], "correlate",
+ [Show, Order, Overlap, Merge]>,
+ HelpText<"Use debug-info or binary correlation to correlate "
+ "profiles with build id fetcher">,
+ MetaVarName<"<mode>">;
+def function : JoinedOrSeparate<["-","--"], "function",
+ [Show, Overlap, Merge]>,
+ HelpText<"Only functions matching the filter are shown or "
+ "merged.">,
+ MetaVarName<"<regex>">;
+
+def weighted_input
+ : JoinedOrSeparate<["-","--"], "weighted-input", [Merge]>,
+ HelpText<"<weight>,<filename>">, MetaVarName<"<weight>,<file>">;
+def binary : Flag<["-","--"], "binary", [Merge]>,
+ HelpText<"Binary encoding">;
+def extbinary : Flag<["-","--"], "extbinary", [Merge]>,
+ HelpText<"Extensible binary encoding (default)">;
+def text : Flag<["-","--"], "text", [Show, Merge]>,
+ HelpText<"Text output format">;
+def gcc : Flag<["-","--"], "gcc", [Merge]>,
+ HelpText<"GCC encoding (only meaningful for -sample)">;
+def input_files
+ : JoinedOrSeparate<["-","--"], "input-files", [Merge]>,
+ HelpText<"Path to file containing newline-separated "
+ "[<weight>,]<filename> entries">,
+ MetaVarName<"<file>">;
+def : JoinedOrSeparate<["-"], "f", [Merge]>, Alias<input_files>,
+ HelpText<"Alias for --input-files">;
+def dump_input_file_list : Flag<["-","--"], "dump-input-file-list", [Merge]>,
+ HelpText<"Dump the list of input files and their "
+ "weights, then exit">,
+ Flags<[HelpHidden]>;
+def remapping_file
+ : JoinedOrSeparate<["-","--"], "remapping-file", [Merge]>,
+ HelpText<"Symbol remapping file">, MetaVarName<"<file>">;
+def : JoinedOrSeparate<["-"], "r", [Merge]>, Alias<remapping_file>,
+ HelpText<"Alias for --remapping-file">;
+def use_md5 : Flag<["-","--"], "use-md5", [Merge]>,
+ HelpText<"Use MD5 to represent strings in the name table "
+ "(only meaningful for -extbinary)">,
+ Flags<[HelpHidden]>;
+def compress_all_sections
+ : Flag<["-","--"], "compress-all-sections", [Merge]>,
+ HelpText<"Compress all sections when writing the profile (only "
+ "meaningful for -extbinary)">,
+ Flags<[HelpHidden]>;
+def sample_merge_cold_context
+ : Flag<["-","--"], "sample-merge-cold-context", [Merge]>,
+ HelpText<"Merge context sample profiles whose count is below cold "
+ "threshold">,
+ Flags<[HelpHidden]>;
+def sample_trim_cold_context
+ : Flag<["-","--"], "sample-trim-cold-context", [Merge]>,
+ HelpText<"Trim context sample profiles whose count is below cold "
+ "threshold">,
+ Flags<[HelpHidden]>;
+def sample_frame_depth_for_cold_context
+ : JoinedOrSeparate<["-","--"], "sample-frame-depth-for-cold-context",
+ [Merge]>,
+ HelpText<"Keep the last K frames while merging cold profile. 1 means "
+ "the context-less base profile">,
+ Flags<[HelpHidden]>, MetaVarName<"<depth>">;
+def output_size_limit
+ : JoinedOrSeparate<["-","--"], "output-size-limit", [Merge]>,
+ HelpText<"Trim cold functions until profile size is below specified "
+ "limit in bytes">,
+ Flags<[HelpHidden]>, MetaVarName<"<bytes>">;
+def gen_partial_profile
+ : Flag<["-","--"], "gen-partial-profile", [Merge]>,
+ HelpText<"Generate a partial profile (only meaningful for -extbinary)">,
+ Flags<[HelpHidden]>;
+def split_layout : Flag<["-","--"], "split-layout", [Merge]>,
+ HelpText<"Split the profile into sections with and without "
+ "inlined functions (only meaningful for "
+ "-extbinary)">,
+ Flags<[HelpHidden]>;
+def supplement_instr_with_sample
+ : JoinedOrSeparate<["-","--"], "supplement-instr-with-sample", [Merge]>,
+ HelpText<"Supplement an instr profile with a sample profile. Output "
+ "will be in instr format.">,
+ Flags<[HelpHidden]>, MetaVarName<"<sample-profile>">;
+def zero_counter_threshold
+ : JoinedOrSeparate<["-","--"], "zero-counter-threshold", [Merge]>,
+ HelpText<"Ratio of zero counters required to drop a function when "
+ "supplementing instr profiles">,
+ Flags<[HelpHidden]>, MetaVarName<"<ratio>">;
+def suppl_min_size_threshold
+ : JoinedOrSeparate<["-","--"], "suppl-min-size-threshold", [Merge]>,
+ HelpText<"Assume functions smaller than this threshold can be inlined "
+ "and will not be adjusted based on sample profile.">,
+ Flags<[HelpHidden]>, MetaVarName<"<n>">;
+def instr_prof_cold_threshold
+ : JoinedOrSeparate<["-","--"], "instr-prof-cold-threshold", [Merge]>,
+ HelpText<"User specified cold threshold for instr profile to override "
+ "the cold threshold from profile summary.">,
+ Flags<[HelpHidden]>, MetaVarName<"<n>">;
+def temporal_profile_trace_reservoir_size
+ : JoinedOrSeparate<["-","--"], "temporal-profile-trace-reservoir-size",
+ [Merge]>,
+ HelpText<"Maximum number of stored temporal profile traces (default: "
+ "100)">,
+ MetaVarName<"<n>">;
+def temporal_profile_max_trace_length
+ : JoinedOrSeparate<["-","--"], "temporal-profile-max-trace-length",
+ [Merge]>,
+ HelpText<"Maximum length of a single temporal profile trace "
+ "(default: 10000)">,
+ MetaVarName<"<n>">;
+def no_function : JoinedOrSeparate<["-","--"], "no-function", [Merge]>,
+ HelpText<"Exclude functions matching the filter from the "
+ "output.">,
+ MetaVarName<"<regex>">;
+def failure_mode
+ : JoinedOrSeparate<["-","--"], "failure-mode", [Merge]>,
+ HelpText<"Failure mode: warn, any, or all">,
+ MetaVarName<"<mode>">;
+def sparse : Flag<["-","--"], "sparse", [Merge]>,
+ HelpText<"Generate a sparse profile (only meaningful for -instr)">;
+def num_threads : JoinedOrSeparate<["-","--"], "num-threads", [Merge]>,
+ HelpText<"Number of merge threads to use (default: autodetect)">,
+ MetaVarName<"<n>">;
+def : JoinedOrSeparate<["-"], "j", [Merge]>, Alias<num_threads>,
+ HelpText<"Alias for --num-threads">;
+def prof_sym_list
+ : JoinedOrSeparate<["-","--"], "prof-sym-list", [Merge]>,
+ HelpText<"Path to file containing the list of function symbols used to "
+ "populate profile symbol list">,
+ MetaVarName<"<file>">;
+def convert_sample_profile_layout
+ : JoinedOrSeparate<["-","--"], "convert-sample-profile-layout", [Merge]>,
+ HelpText<"Convert the generated profile to a new layout: nest or flat">,
+ MetaVarName<"<layout>">;
+def drop_profile_symbol_list
+ : Flag<["-","--"], "drop-profile-symbol-list", [Merge]>,
+ HelpText<"Drop the profile symbol list when merging AutoFDO profiles "
+ "(only meaningful for -sample)">,
+ Flags<[HelpHidden]>;
+def keep_vtable_symbols
+ : Flag<["-","--"], "keep-vtable-symbols", [Merge]>,
+ HelpText<"Keep the vtable symbols in indexed profiles">,
+ Flags<[HelpHidden]>;
+def write_prev_version
+ : Flag<["-","--"], "write-prev-version", [Merge]>,
+ HelpText<"Write the previous version of indexed format for forward "
+ "compatibility.">,
+ Flags<[HelpHidden]>;
+def memprof_version
+ : JoinedOrSeparate<["-","--"], "memprof-version", [Merge]>,
+ HelpText<"Specify the version of the memprof format to use (2, 3, or 4)">,
+ Flags<[HelpHidden]>, MetaVarName<"<n>">;
+def memprof_full_schema
+ : Flag<["-","--"], "memprof-full-schema", [Merge]>,
+ HelpText<"Use the full schema for serialization">,
+ Flags<[HelpHidden]>;
+def memprof_random_hotness
+ : Flag<["-","--"], "memprof-random-hotness", [Merge]>,
+ HelpText<"Generate random hotness values">,
+ Flags<[HelpHidden]>;
+def memprof_random_hotness_seed
+ : JoinedOrSeparate<["-","--"], "memprof-random-hotness-seed", [Merge]>,
+ HelpText<"Random hotness seed to use (0 to generate new seed)">,
+ Flags<[HelpHidden]>, MetaVarName<"<n>">;
+
+def similarity_cutoff
+ : JoinedOrSeparate<["-","--"], "similarity-cutoff", [Overlap]>,
+ HelpText<"List overlapped functions with similarities below the cutoff "
+ "(percentage times 10000).">,
+ MetaVarName<"<n>">;
+def cs : Flag<["-","--"], "cs", [Overlap]>,
+ HelpText<"For context sensitive PGO counts. Does not work with "
+ "CSSPGO.">;
+def value_cutoff
+ : JoinedOrSeparate<["-","--"], "value-cutoff", [Show, Overlap]>,
+ HelpText<"Cutoff value used for filtering. Meaning depends on subcommand">,
+ MetaVarName<"<n>">;
+
+def counts : Flag<["-","--"], "counts", [Show]>,
+ HelpText<"Show counter values for shown functions">;
+def show_format
+ : JoinedOrSeparate<["-","--"], "show-format", [Show]>,
+ HelpText<"Emit output in the selected format: text, json, or yaml">,
+ MetaVarName<"<format>">;
+def json : Flag<["-","--"], "json", [Show]>,
+ HelpText<"Show sample profile data in JSON format "
+ "(deprecated, use --show-format=json)">;
+def ic_targets : Flag<["-","--"], "ic-targets", [Show]>,
+ HelpText<"Show indirect call site target values">;
+def show_vtables : Flag<["-","--"], "show-vtables", [Show]>,
+ HelpText<"Show vtable names for shown functions">;
+def memop_sizes : Flag<["-","--"], "memop-sizes", [Show]>,
+ HelpText<"Show profiled sizes of memory intrinsic calls">;
+def detailed_summary : Flag<["-","--"], "detailed-summary", [Show]>,
+ HelpText<"Show detailed profile summary">;
+def detailed_summary_cutoffs
+ : CommaJoined<["-","--"], "detailed-summary-cutoffs", [Show]>,
+ HelpText<"Cutoff percentages (times 10000) for generating detailed "
+ "summary">,
+ MetaVarName<"<list>">;
+def hot_func_list : Flag<["-","--"], "hot-func-list", [Show]>,
+ HelpText<"Show profile summary of a list of hot functions">;
+def all_functions : Flag<["-","--"], "all-functions", [Show]>,
+ HelpText<"Details for each and every function">;
+def showcs : Flag<["-","--"], "showcs", [Show]>,
+ HelpText<"Show context sensitive counts">;
+def topn : JoinedOrSeparate<["-","--"], "topn", [Show]>,
+ HelpText<"Show the list of functions with the largest internal counts">,
+ MetaVarName<"<n>">;
+def list_below_cutoff
+ : Flag<["-","--"], "list-below-cutoff", [Show]>,
+ HelpText<"Only output names of functions whose max count values are "
+ "below the cutoff value">;
+def show_prof_sym_list
+ : Flag<["-","--"], "show-prof-sym-list", [Show]>,
+ HelpText<"Show profile symbol list if it exists in the profile.">;
+def show_sec_info_only
+ : Flag<["-","--"], "show-sec-info-only", [Show]>,
+ HelpText<"Show the information of each section in the sample profile "
+ "(extbinary sample profiles only)">;
+def binary_ids : Flag<["-","--"], "binary-ids", [Show]>,
+ HelpText<"Show binary ids in the profile.">;
+def temporal_profile_traces
+ : Flag<["-","--"], "temporal-profile-traces", [Show]>,
+ HelpText<"Show temporal profile traces in the profile.">;
+def covered : Flag<["-","--"], "covered", [Show]>,
+ HelpText<"Show only the functions that have been executed.">;
+def profile_version : Flag<["-","--"], "profile-version", [Show]>,
+ HelpText<"Show profile version.">;
+
+def num_test_traces
+ : JoinedOrSeparate<["-","--"], "num-test-traces", [Order]>,
+ HelpText<"Keep aside the last <num-test-traces> traces when computing "
+ "function order to evaluate that order">,
+ MetaVarName<"<n>">;
+
+def fs_discriminator_pass
+ : JoinedOrSeparate<["-","--"], "fs-discriminator-pass",
+ [Show, Overlap, Merge]>,
+ HelpText<"Zero out the discriminator bits for the FS discriminator "
+ "pass beyond this value.">,
+ Flags<[HelpHidden]>, MetaVarName<"<pass>">;
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 74c4732ca129a..592449729ecc4 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -12,11 +12,18 @@
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/Debuginfod/HTTPClient.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/Object/Binary.h"
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/OptTable.h"
+#include "llvm/Option/Option.h"
#include "llvm/ProfileData/DataAccessProf.h"
#include "llvm/ProfileData/InstrProfCorrelator.h"
#include "llvm/ProfileData/InstrProfReader.h"
@@ -35,11 +42,12 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormattedStream.h"
-#include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/LLVMDriver.h"
#include "llvm/Support/MD5.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Regex.h"
+#include "llvm/Support/StringSaver.h"
#include "llvm/Support/ThreadPool.h"
#include "llvm/Support/Threading.h"
#include "llvm/Support/VirtualFileSystem.h"
@@ -47,35 +55,15 @@
#include "llvm/Support/raw_ostream.h"
#include <algorithm>
#include <cmath>
+#include <limits>
#include <optional>
+#include "Opts.inc"
+
using namespace llvm;
+using namespace llvm::opt;
using ProfCorrelatorKind = InstrProfCorrelator::ProfCorrelatorKind;
-// https://llvm.org/docs/CommandGuide/llvm-profdata.html has documentations
-// on each subcommand.
-cl::SubCommand ShowSubcommand(
- "show",
- "Takes a profile data file and displays the profiles. See detailed "
- "documentation in "
- "https://llvm.org/docs/CommandGuide/llvm-profdata.html#profdata-show");
-cl::SubCommand OrderSubcommand(
- "order",
- "Reads temporal profiling traces from a profile and outputs a function "
- "order that reduces the number of page faults for those traces. See "
- "detailed documentation in "
- "https://llvm.org/docs/CommandGuide/llvm-profdata.html#profdata-order");
-cl::SubCommand OverlapSubcommand(
- "overlap",
- "Computes and displays the overlap between two profiles. See detailed "
- "documentation in "
- "https://llvm.org/docs/CommandGuide/llvm-profdata.html#profdata-overlap");
-cl::SubCommand MergeSubcommand(
- "merge",
- "Takes several profiles and merge them together. See detailed "
- "documentation in "
- "https://llvm.org/docs/CommandGuide/llvm-profdata.html#profdata-merge");
-
namespace {
enum ProfileKinds { instr, sample, memory };
enum FailureMode { warnOnly, failIfAnyAreInvalid, failIfAllAreInvalid };
@@ -90,408 +78,132 @@ enum ProfileFormat {
};
enum class ShowFormat { Text, Json, Yaml };
-} // namespace
-// Common options.
-cl::opt<std::string> OutputFilename("output", cl::value_desc("output"),
- cl::init("-"), cl::desc("Output file"),
- cl::sub(ShowSubcommand),
- cl::sub(OrderSubcommand),
- cl::sub(OverlapSubcommand),
- ...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
dzbarsky
force-pushed
the
llvm-profdata
branch
from
January 25, 2026 19:09
3cf9452 to
544efd7
Compare
dzbarsky
requested review from
aaronmondal,
keith and
rupprecht
as code owners
January 25, 2026 19:09
dzbarsky
force-pushed
the
llvm-profdata
branch
3 times, most recently
from
January 25, 2026 19:26
8d66f33 to
481b747
Compare
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) LLVMLLVM.tools/llvm-profdata/sample-fs.testIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🪟 Windows x64 Test Results
Failed Tests(click on a test name to see its output) LLVMLLVM.tools/llvm-profdata/invalid-profdata.testLLVM.tools/llvm-profdata/sample-fs.testLLVM.tools/llvm-profdata/weight-instr.testLLVM.tools/llvm-profdata/weight-sample.testIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
dzbarsky
force-pushed
the
llvm-profdata
branch
2 times, most recently
from
January 25, 2026 19:56
6cf348d to
d49e25b
Compare
Member
petrhosek
reviewed
Jan 26, 2026
| return 1; | ||
| } | ||
|
|
||
| cl::ParseCommandLineOptions(argc, argv, "LLVM profile data\n"); | ||
| StringRef Subcommand = argv[1]; |
Member
There was a problem hiding this comment.
This should be using Args.getSubCommand, see
llvm-project/llvm/examples/OptSubcommand/llvm-hello-sub.cpp
Lines 81 to 82 in a53daac
petrhosek
reviewed
Jan 26, 2026
Comment on lines
+3573
to
+3576
| if (Subcommand == "--help" || Subcommand == "-help" || Subcommand == "-h") { | ||
| printTopLevelHelp(); | ||
| return 0; | ||
| } |
Member
There was a problem hiding this comment.
This should be using OptTable support as well, see
Contributor
Author
|
@petrhosek Thanks for taking a look - I updated per your comments in a separate commit but not sure if I did it properly, mind taking a look to see if it's what you intended? |
dzbarsky
force-pushed
the
llvm-profdata
branch
2 times, most recently
from
January 26, 2026 21:19
68648b9 to
5da6057
Compare
Contributor
|
@dzbarsky -- thank you for the PR. I am interested in reviewing this patch but will be a few days before I can get to this as I am returning from a break. In the meanwhile can you take a look at why the builders are failing? There is no regression tests currently for llvm-profdata --help and for other subcommand help text outputs. Can you also consider adding tests for the help text outputs and/or comment on whether you are seeing the behavior of the tool including help text to be correct after these changes? Thank you. |
Contributor
|
CC: @mingmingl-llvm @teresajohnson -- this PR may be of interest to you. |
dzbarsky
force-pushed
the
llvm-profdata
branch
3 times, most recently
from
January 27, 2026 20:55
c402d4a to
2b4e33a
Compare
Contributor
Author
No worries, I'll be traveling later this week/weekend to give a talk about LLVM at Fosdem, so I'll also be delayed :) It looks like CI discovered that there are some cl::Opt definitions in libs that did not get ported; for now I kept a limited form of CL parsing for those, we will need to fix all upstream tools that use them (probably do their own parsing and thread through options), but it feels like too big a change for a single PR. Let mek now if you have any better ideas! Added some tests for the help outputs. |
dzbarsky
force-pushed
the
llvm-profdata
branch
from
January 27, 2026 21:32
2b4e33a to
1b7e425
Compare
ellishg
reviewed
Jan 30, 2026
Comment on lines
612
to
619
| static std::string getFuncName(const SampleProfileMap::value_type &Val) { | ||
| return Val.second.getContext().toString(); | ||
| } | ||
|
|
||
| template <typename T> | ||
| static void filterFunctions(T &ProfileMap) { | ||
| template <typename T> static void filterFunctions(T &ProfileMap) { | ||
| bool hasFilter = !FuncNameFilter.empty(); | ||
| bool hasNegativeFilter = !FuncNameNegativeFilter.empty(); | ||
| if (!hasFilter && !hasNegativeFilter) |
Contributor
There was a problem hiding this comment.
These formatting changes can be reverted to preserve blame lines.
Contributor
Author
There was a problem hiding this comment.
I can try, but I think clang-format insisted on these and CI was red without it. Perhaps the clang-format check got introduced or reconfigured after this code was written?
Member
There was a problem hiding this comment.
If that's the case, the usual recommendation is to reformat the file first as a separate change to minimize the size of the PR.
Contributor
There was a problem hiding this comment.
If you use git clang-format then it will only attempt to format lines that you have touched, and will leave the other lines (like this) alone.
https://clang.llvm.org/docs/ClangFormat.html#git-integration
Comment on lines
1204
to
1224
| @@ -1522,12 +1219,8 @@ remapSamples(const sampleprof::FunctionSamples &Samples, | |||
| } | |||
|
|
|||
| static sampleprof::SampleProfileFormat FormatMap[] = { | |||
| sampleprof::SPF_None, | |||
| sampleprof::SPF_Text, | |||
| sampleprof::SPF_None, | |||
| sampleprof::SPF_Ext_Binary, | |||
| sampleprof::SPF_GCC, | |||
| sampleprof::SPF_Binary}; | |||
| sampleprof::SPF_None, sampleprof::SPF_Text, sampleprof::SPF_None, | |||
| sampleprof::SPF_Ext_Binary, sampleprof::SPF_GCC, sampleprof::SPF_Binary}; | |||
|
|
|||
Contributor
There was a problem hiding this comment.
Same here. These formatting changes can be reverted.
Comment on lines
+3198
to
+3202
| StringRef V = getOptionValue(A); | ||
| if (V.empty()) { | ||
| Value = true; | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| StringRef V = getOptionValue(A); | |
| if (V.empty()) { | |
| Value = true; | |
| return true; | |
| } | |
| StringRef V = getOptionValue(A, /*default=*/"true"); |
We can simplify this logic if we add a default parameter to getOptionValue().
| .Case("PassLast", FSDiscriminatorPass::PassLast) | ||
| .Case("pass-last", FSDiscriminatorPass::PassLast) | ||
| .Case("passlast", FSDiscriminatorPass::PassLast) | ||
| .Default(std::nullopt); |
Contributor
There was a problem hiding this comment.
What is the motivation behind using Opt flag parsing instead of cl::opt? To me this makes options more complicated. For this option, we need to write a whole new function to parse with custom enum values. This leaves opportunity for bugs to creep in.
With cl::opt, enum values are right next to the option definition and there is no need to write custom option parsing code.
llvm-project/llvm/tools/llvm-profdata/llvm-profdata.cpp
Lines 1176 to 1185 in 48c6c7f
Contributor
Author
There was a problem hiding this comment.
As I understand it, cl::opt "pollutes" the help text by registering flags at static scope, which causes confusion when binaries are linked together into a busybox/multicall driver binary. See 3f52eef for additional context.
I do agree with you that the end code is not necessarily more pleasing, and perhaps cl::opt could have been modified to be compatible with multicall. It seems unfortunate to have multiple flag parsing libraries in the repo without a clear winner.
Member
There was a problem hiding this comment.
There are other reasons beyond the scoping, OptTable is more powerful and generally a better fit for user facing tools (this is especially true when you're trying to match option spelling of existing tools), cl::opt is primarily intended for internal options (e.g. pass flags). https://discourse.llvm.org/t/rfc-llvm-busybox-proposal/58494 more context.
petrhosek
reviewed
Jan 30, 2026
dzbarsky
force-pushed
the
llvm-profdata
branch
6 times, most recently
from
February 6, 2026 14:19
1694628 to
064c27e
Compare
dzbarsky
force-pushed
the
llvm-profdata
branch
from
February 6, 2026 15:06
064c27e to
edf446c
Compare
Contributor
Author
@petrhosek Thanks for sharing those, I've taken a stab at reworking this PR to align with the semantic flag changes made in the other ones, such as dropping the single-dash long flags for consistency, removing Mind taking another look at the llvm-profdata main and let me know if you see additional opportunities to use OptTable better? Would love to get this merged! |
dzbarsky
commented
Feb 6, 2026
| return true; | ||
| } | ||
|
|
||
| static bool parseFSDiscriminatorPassArg(const opt::InputArgList &Args) { |
Contributor
Author
There was a problem hiding this comment.
not sure if OptTable has a builtin way to model this?
Contributor
There was a problem hiding this comment.
I think something like this could work: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Options/Options.td#L9814-L9822
We could combine Values, ValuesCode:
Can you try this?
def fs_discriminator_pass : Joined<["-"], "fs-discriminator-pass=">,
Group<f_Group>,
HelpText<"Set the FS discriminator pass">,
Values<"base,pass1,pass2,pass3,passlast">,
NormalizedValuesScope<"FSDiscriminatorPass">,
MarshallingInfoEnum<"Opts.FSDiscriminatorPass", "Base">,
ValuesCode<[{
return llvm::StringSwitch(Value)
.Case("base", FSDiscriminatorPass::Base)
.Case("pass1", FSDiscriminatorPass::Pass1)
.Case("pass2", FSDiscriminatorPass::Pass2)
.Case("pass3", FSDiscriminatorPass::Pass3)
.Case("passlast", FSDiscriminatorPass::PassLast)
.Default(FSDiscriminatorPass::Base);
}]>;
If the above doesn't work we could still simplify this with the following:
def fs_discriminator_pass : Joined<["-"], "fs-discriminator-pass=">,
Values<"base,pass1,pass2,pass3,passlast">,
HelpText<"Set the FS discriminator pass">;
You can then probably use Value.lowercase() in your StringSwitch check.
| return llvm::is_contained(OtherPositionals, Flag) || RawFirstArg == Flag; | ||
| }; | ||
|
|
||
| if (Args.hasArg(OPT_help) || HasRawFlag("--help")) { |
Contributor
Author
There was a problem hiding this comment.
even as top-level options I was seeing these not resolving if no command was selected, so needed this workaround. Maybe I missed something?
Contributor
There was a problem hiding this comment.
Can you provide an example invocation and what case is not being handled correctly?
IIUC you are trying to handle the help flag here. You should be passing the helptext from your Opts.td file which you seem to be doing. So I cant fathom what is the purpose of this help text handling. If the library is not handling this correctly we must fix that first anyway.
Contributor
Author
There was a problem hiding this comment.
I debugged this a bit, the problem was Tbl.parseArgs(argc - 1, argv + 1, OPT_UNKNOWN, Saver, [&](StringRef Msg) { which was dropping the first arg (--help). thanks for the prod
Prabhuk
requested changes
Feb 13, 2026
| return llvm::is_contained(OtherPositionals, Flag) || RawFirstArg == Flag; | ||
| }; | ||
|
|
||
| if (Args.hasArg(OPT_help) || HasRawFlag("--help")) { |
Contributor
There was a problem hiding this comment.
Can you provide an example invocation and what case is not being handled correctly?
IIUC you are trying to handle the help flag here. You should be passing the helptext from your Opts.td file which you seem to be doing. So I cant fathom what is the purpose of this help text handling. If the library is not handling this correctly we must fix that first anyway.
dzbarsky
force-pushed
the
llvm-profdata
branch
5 times, most recently
from
February 16, 2026 02:19
008a1a8 to
205ed1a
Compare
Contributor
Author
|
@Prabhuk I've addressed your feedback, please take another look |
Prabhuk
reviewed
Feb 22, 2026
| @@ -10,13 +10,20 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "llvm/ADT/ArrayRef.h" | |||
Contributor
There was a problem hiding this comment.
I believe this header include is not needed. Please check if this header include and others from ADT are necessary and remove if not needed:
https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible
Comment on lines
+686
to
+687
| const uint64_t TraceReservoirSize = TemporalProfTraceReservoirSize; | ||
| const uint64_t MaxTraceLength = TemporalProfMaxTraceLength; |
Contributor
There was a problem hiding this comment.
These local variables are not used. Please remove.
| @@ -3424,7 +3122,7 @@ static int order_main() { | |||
| ArrayRef Traces = Reader->getTemporalProfTraces(); | |||
| if (NumTestTraces && NumTestTraces >= Traces.size()) | |||
| exitWithError( | |||
| "--" + NumTestTraces.ArgStr + | |||
| "--num-test-traces" | |||
Contributor
There was a problem hiding this comment.
Can you include this line into the string literal in the following line and remove this line?
| return true; | ||
| } | ||
|
|
||
| static bool parseFSDiscriminatorPassArg(const opt::InputArgList &Args) { |
Contributor
There was a problem hiding this comment.
I think something like this could work: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Options/Options.td#L9814-L9822
We could combine Values, ValuesCode:
Can you try this?
def fs_discriminator_pass : Joined<["-"], "fs-discriminator-pass=">,
Group<f_Group>,
HelpText<"Set the FS discriminator pass">,
Values<"base,pass1,pass2,pass3,passlast">,
NormalizedValuesScope<"FSDiscriminatorPass">,
MarshallingInfoEnum<"Opts.FSDiscriminatorPass", "Base">,
ValuesCode<[{
return llvm::StringSwitch(Value)
.Case("base", FSDiscriminatorPass::Base)
.Case("pass1", FSDiscriminatorPass::Pass1)
.Case("pass2", FSDiscriminatorPass::Pass2)
.Case("pass3", FSDiscriminatorPass::Pass3)
.Case("passlast", FSDiscriminatorPass::PassLast)
.Default(FSDiscriminatorPass::Base);
}]>;
If the above doesn't work we could still simplify this with the following:
def fs_discriminator_pass : Joined<["-"], "fs-discriminator-pass=">,
Values<"base,pass1,pass2,pass3,passlast">,
HelpText<"Set the FS discriminator pass">;
You can then probably use Value.lowercase() in your StringSwitch check.
dzbarsky
force-pushed
the
llvm-profdata
branch
from
February 27, 2026 08:24
0e03fd2 to
91b9172
Compare
Contributor
Author
|
@Prabhuk thanks for taking a look, I couldn't get the |
petrhosek
reviewed
Feb 27, 2026
Comment on lines
3248
to
3252
Member
There was a problem hiding this comment.
Using one option parser only to construct a string and feed it to another option parser just adds unnecessary complexity. Can you instead parse these options directly just like the other ones?
Contributor
Author
There was a problem hiding this comment.
I agree it's a code smell; the reason for it is that those cl::Opt are defined deeper in a library instead of in the main file, and are used by other tools as well. Given the size of this change, I felt that untangling that would be better left to a subsequent PR
Member
|
I'd like to avoid changing all the tests and also potentially breaking downstream users who rely on the old flag spelling. I also don't think it's necessary. For example, rather than changing all instances of |
Contributor
Author
OK, I can undo that, that's what I had done originally but then changed to the current approach based off your comment here #177868 (comment) since the other PRs also made similar changes. I'll revert these bits, but it would be great to avoid more confusion/ping-ponging here, especially given the time between review cycles :) |
Contributor
Author
|
@petrhosek Do you also want to support |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Similar to https://lists.llvm.org/pipermail/llvm-dev/2021-July/151622.html
"Binary utilities: switch command line parsing from llvm::cl to OptTable"
Users should generally observe no difference as long as they only use intended
option forms. Behavior changes:
Motivation: So we can undo 3f52eef and bring it back into the driver